Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: fix test-cli-syntax assertions on windows #12212

Merged
merged 1 commit into from
Apr 4, 2017
Merged

test: fix test-cli-syntax assertions on windows #12212

merged 1 commit into from
Apr 4, 2017

Conversation

not-an-aardvark
Copy link
Contributor

The test introduced in a5f91ab accidentally introduced failures on some windows builds. Update the assertion that was causing the failures.

Ref: #11689

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 4, 2017
@not-an-aardvark
Copy link
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/7194/

@not-an-aardvark not-an-aardvark mentioned this pull request Apr 4, 2017
Closed
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Apr 4, 2017

We should land this ASAP (once CI passes) to fix CI on windows master.

cc/ @nodejs/platform-windows

@refack
Copy link
Contributor

refack commented Apr 4, 2017 •
edited
Loading

I hate the \r\n EOF mess.

@bzoz
Copy link
Contributor

bzoz commented Apr 4, 2017

LGTM if the CI is green

@Trott
Copy link
Member

Trott commented Apr 4, 2017

This LGTM as-is, although I think I'd prefer assert.strictEqual(c.stderr.trim(), ...);. That would provide a more helpful message in the event that the assertion fires.

@not-an-aardvark
Copy link
Contributor Author

not-an-aardvark commented Apr 4, 2017 •
edited
Loading

@Trott I agree that would have been better, but for now I think I'm going to land this as-is so that the build on master can be fixed ASAP. I can follow up with another PR to improve the error message.

@not-an-aardvark
test: fix test-cli-syntax assertions on windows
9348f31 
The test introduced in a5f91ab
accidentally introduced failures on some windows builds. Update the
assertion that was causing the failures.

PR-URL: nodejs#12212
Ref: nodejs#11689
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@not-an-aardvark not-an-aardvark merged commit 9348f31 into nodejs:master Apr 4, 2017
@not-an-aardvark
Copy link
Contributor Author

Landed in 9348f31

@not-an-aardvark not-an-aardvark deleted the fix-windows-cli-test branch April 4, 2017 16:16
@gibfahn gibfahn mentioned this pull request Apr 4, 2017
Closed
4 tasks
@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
@italoacasas
Copy link
Contributor

cc @not-an-aardvark

@not-an-aardvark
Copy link
Contributor Author

This fixes a bug that was introduced in a semver-major commit -- it doesn't seem like it makes sense to backport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@not-an-aardvark @gibfahn @refack @bzoz @Trott @italoacasas @jasnell @nodejs-github-bot